feat: Add option to select url schema (notion: or https:) #642
Conversation
WalkthroughThis pull request introduces a new URL schema preference for Notero, allowing users to choose between Changes
Sequence DiagramsequenceDiagram
participant User
participant Preferences
participant SyncItem
participant NoteroPref
User->>Preferences: Select URL Schema
Preferences->>NoteroPref: Save urlSchema Preference
SyncItem->>NoteroPref: Get urlSchema Preference
alt Preference is notion://
SyncItem->>SyncItem: Convert to notion:// URL
else Preference is https://
SyncItem->>SyncItem: Use original URL
end
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/content/prefs/preferences.xhtml (1)
87-91: Consider adding disabled state handling.Other menulists in the file (e.g.,
notero-notionDatabase) have a disabled state. For consistency, consider adding the same behavior here if the URL schema should be disabled under certain conditions (e.g., when not connected to Notion).<menulist id="notero-urlSchema" + disabled="true" native="true" preference="extensions.notero.urlSchema" >src/content/sync/sync-regular-item.ts (1)
32-37: Simplify the URL schema selection logic.The current implementation is correct but can be made more concise:
- // Decide link schema based on user preference - const notionURL = - getNoteroPref(NoteroPref.urlSchema) === UrlSchema.notion - ? convertWebURLToAppURL(response.url) - : response.url; + const notionURL = getNoteroPref(NoteroPref.urlSchema) === UrlSchema.notion + ? convertWebURLToAppURL(response.url) + : response.url;src/locale/en-US/notero.ftl (1)
31-31: Enhance the description to better guide users.The current description could be more helpful by explicitly stating the benefits and tradeoffs of each choice:
-notero-preferences-links-groupbox-description = Define if links are saved as notion:// or https:// links. Note that notion:// links are only accessible if the desktop app is installed. +notero-preferences-links-groupbox-description = Choose between notion:// or https:// links. notion:// links open directly in the desktop app for a seamless experience but require the app to be installed. https:// links work universally but always open in the browser first.src/content/prefs/notero-pref.ts (3)
23-26: Add JSDoc comments to document URL schema options.Consider adding documentation to explain each schema option and its implications:
+/** URL schema options for Notion links */ export enum UrlSchema { + /** Opens links directly in Notion desktop app but requires app installation */ notion = 'notion', + /** Opens links in browser first, works universally */ https = 'https', }
66-70: Strengthen type checking in getUrlSchemaPref.The current implementation could be more strict in its type validation:
function getUrlSchemaPref(value: Zotero.Prefs.Value): UrlSchema | undefined { - return Object.values(UrlSchema).includes(value as UrlSchema) - ? (value as UrlSchema) - : undefined; + return typeof value === 'string' && Object.values(UrlSchema).includes(value as UrlSchema) + ? (value as UrlSchema) + : undefined; }
23-26: Reconsider the default URL schema choice.Currently, the default schema is set to
notion://. While this provides a seamless experience for desktop app users, it might cause confusion for users who don't have the desktop app installed. Consider:
- Making
https://the default for universal compatibility- Detecting desktop app presence to auto-select the appropriate schema
- Adding a first-run wizard to help users make an informed choice
src/content/prefs/preferences.tsx (2)
87-87: Consider async initialization consistency.While the initialization follows the existing pattern, consider making
initUrlSchemaMenuasync for consistency withinitPageTitleFormatMenu. This would allow for potential future async operations like loading saved preferences or validating schema options.Also applies to: 96-96
116-126: Consider enhancing user guidance for URL schema selection.Regarding the specific feedback points from the PR:
The default choice of
notion://is reasonable as it provides native app integration, but consider:
- Adding a comment in the code explaining this default choice
- Documenting the implications of each choice in the UI
- Potentially making this choice during first-time setup
To improve explanations in preferences:
- Add help text explaining when to use each schema
- Include examples of how links will appear
- Consider adding a link to documentation
Example help text structure:
notion://: Opens links in the Notion desktop app (recommended for desktop users)https://: Opens links in web browser (recommended for users without the desktop app)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/content/prefs/notero-pref.ts(5 hunks)src/content/prefs/preferences.tsx(5 hunks)src/content/prefs/preferences.xhtml(1 hunks)src/content/sync/sync-regular-item.ts(2 hunks)src/locale/en-US/notero.ftl(1 hunks)
🔇 Additional comments (3)
src/content/prefs/preferences.tsx (3)
21-25: LGTM! Clean import organization.The import statement is well-organized and properly grouped with related preference imports.
64-64: LGTM! Property declaration follows established patterns.The urlSchemaMenu property is properly typed and follows the existing pattern of other menu element declarations in the class.
116-126: 🛠️ Refactor suggestionSeveral improvements needed for URL schema initialization.
The implementation could be enhanced in the following areas:
- Load the default value from saved preferences instead of hardcoding
- Add localization for schema labels
- Add JSDoc documentation explaining the purpose and behavior
- Add tooltips or help text explaining the implications of each schema choice
Here's a suggested implementation:
+ /** + * Initializes the URL schema selection menu. + * This allows users to choose between notion:// and https:// URL formats + * for Notion links. + */ private initUrlSchemaMenu(): void { const menuItems = Object.values(UrlSchema).map<MenuItem>((schema) => ({ disabled: false, - label: `${schema}://`, + l10nId: `notero-preferences-url-schema-${schema}`, value: schema, })); setMenuItems(this.urlSchemaMenu, menuItems); this.urlSchemaMenu.disabled = false; - this.urlSchemaMenu.value = UrlSchema.notion; // Set default value to Notion schema + // Load saved preference or fall back to default + this.urlSchemaMenu.value = Zotero.Prefs.get('extensions.notero.urlSchema') || UrlSchema.notion; + + // Add tooltip explaining the implications + document.l10n.setAttributes( + this.urlSchemaMenu, + 'notero-preferences-url-schema-tooltip' + ); }This implementation addresses the PR objectives by:
- Maintaining the default of "notion:" schema while making it configurable
- Improving explanations through localized labels and tooltips
Let's verify if the necessary L10N strings are defined:
dvanoni
left a comment
dvanoni
left a comment
There was a problem hiding this comment.
@dgruano, thanks for opening this PR! I appreciate your patience as it took me a while to get around to looking at it.
Some thoughts regarding your questions:
I set the current default to the notion schema. Is that good?
I wonder if it makes more sense to default to https since that should work for everyone regardless of whether they have the Notion desktop app installed. It seems like a "safer" option to me.
We may want to improve the explanation in the preference section. Any suggestion?
I imagine most users won't know what a "URL scheme" is. What if we update the explanation to something like this:
Select the format for Notion links saved on each item. The Notion desktop app must be installed to open desktop links.
And change the label to Notion Link Format:
And change the options to:
WebDesktop
| pageTitleFormat = 'pageTitleFormat', | ||
| syncNotes = 'syncNotes', | ||
| syncOnModifyItems = 'syncOnModifyItems', | ||
| urlSchema = 'urlSchema', |
There was a problem hiding this comment.
I'd like to prefix the pref name with notion so that we know which URLs we're talking about.
Also, the correct term is scheme rather than schema, so we'll want to update that everywhere.
| urlSchema = 'urlSchema', | |
| notionUrlScheme = 'notionUrlScheme', |
|
|
||
| setMenuItems(this.urlSchemaMenu, menuItems); | ||
| this.urlSchemaMenu.disabled = false; | ||
| this.urlSchemaMenu.value = UrlSchema.notion; // Set default value to Notion schema |
There was a problem hiding this comment.
This line doesn't appear to be working for me.
I think we'll instead want to set the default in src/prefs.js.
2 participants
Hi there!
This PR should address the feature request in #53.
I added a new field in the Notero preferences page that allows the user to choose whether to use https or notion url schema. Whenever an item is created, this preference is checked and the url is build accordingly. Interestingly, if the user changes the option, all URLs will be updated to match the desired schema.
I would really appreciate your opinion on two things @dvanoni :
Happy to hear your thoughts!
Summary by CodeRabbit
New Features
Documentation
User Interface